Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increase accuracy of power iterations and remove flag normalize_rho #86

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

JakobAsslaender
Copy link
Member

Hi @tknopp,

I increased the default accuracy of the power iterations to ensure the correction step size of FISTA, optISTA, POGM reconstuctions.

I also changed the interface by removing the normalize_rho flag and building the power iterations into the default call of rho, increasing the flexibility to modify the settings of the power iterations as needed.

Any chance you could give me a quick thumbs up on this, I would like to go ahead and release this code, as we want to start a bunch of recons with it, and I would prefer not doing this on branch that less permanence than are release.

Many thanks and have a great weekend!

@JakobAsslaender JakobAsslaender requested a review from tknopp June 28, 2024 16:06
Copy link
Member Author

@JakobAsslaender JakobAsslaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nHackel: not sure what the problem with the new CI setup is, can you check? The package passed all tests locally.

@nHackel
Copy link
Member

nHackel commented Jun 28, 2024

As far as I can see the failing CI is just buildkite. That is CI that runs on GPUs, which wouldnt work for the master anyway atm.

@nHackel
Copy link
Member

nHackel commented Jun 28, 2024

Ah and it fails because the master branch doesnt have the config yet.

From a CI standpoint you can feel free ro merge. If this is merged to the master I can also do the merge with the gpuExtension branch. Or do you want to do GPU recos with this?

@JakobAsslaender
Copy link
Member Author

Right now we want to run recons on the CPU, but we are working on the GPU implementation in parallel, so yes, we do.

Ok, then I am just waiting for either @tknopp or you to give me a thumbs up content-wise.

@tknopp
Copy link
Member

tknopp commented Jun 28, 2024

Looks good to me!

@JakobAsslaender JakobAsslaender merged commit e4c43ea into master Jun 28, 2024
8 of 9 checks passed
@JakobAsslaender JakobAsslaender deleted the PowerIterations_accuracy branch June 28, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants